Skip to content

fix: wire persisted provider configuration into runtime - #3299

Merged
Yeachan-Heo merged 2 commits into
ultraworkers:mainfrom
D4B34R987:main
Oct 12, 2026
Merged

Yeachan-Heo merged 2 commits into
ultraworkers:mainfrom
D4B34R987:main

Conversation

@D4B34R987

Copy link
Copy Markdown
Contributor

Wires persisted provider configuration from runtime settings into actual provider execution. Provider kind is authoritative when explicitly configured, with regression coverage for Anthropic, xAI, OpenAI, DashScope, and local OpenAI-compatible routing. Validation completed with cargo fmt --check, runtime/API/CLI test suites, and git diff --check.

@XinwangTao

Copy link
Copy Markdown

Good change — making provider kind authoritative removes the ambiguity of a persisted config silently overriding the routing. One question for clarity: with kind authoritative, what wins against an explicit runtime hint, e.g. a --provider flag or a model prefix on the command line? If the stored kind always wins regardless of the current invocation, a user doing a one-off provider switch could be surprised. Either a short precedence note in the docs or a test covering flag-vs-persisted precedence would make the intent unambiguous.

@XinwangTao

Copy link
Copy Markdown

补充建议形成闭环:优先级建议定为「显式运行期参数(--provider / 命令行 model 前缀)> 持久化配置中的 provider kind > 默认推断路由」。这样持久化配置保证会话间一致,命令行参数保留单次覆盖能力,符合多数 CLI 惯例。若同意此语义,可补一条 flag-vs-persisted 的优先级回归测试;runtime 测试套件这边需要的话我可以协助补。

@XinwangTao

Copy link
Copy Markdown

5adb8be (fix: honor explicit model provider over persisted config) 正好落实了我上次建议的优先级语义——显式运行期参数优先于持久化 provider kind,符合 CLI 惯例,赞。合入前一个小建议:可以合并掉那批临时的 CI patcher / stage / export commits(82eae82~29c904a),用一个干净的功能 commit 承载 5adb8be 的改动,评审与 bisect 都会更清爽。

@XinwangTao

Copy link
Copy Markdown

Re-checked at 5adb8be — the "explicit args > persisted kind > default inference" priority is now correctly applied through the injection path, matching what we discussed; the fallback chain reads clean. Also noticed the temporary PR3299 CI hook has been reverted (530a37f), good cleanup so it won't pollute the final merge. Worth re-running the full CI once more on the cleaned head before merge to confirm nothing regressed.

@XinwangTao

Copy link
Copy Markdown

Reviewed the force-push to f8ad763 — the head stays consistent with the "explicit args > persisted kind > default inference" semantics we settled on, and the temp CI hook is out of the final diff. No new concerns; please run the full CI once on this cleaned head before merge and we can get this in.

@XinwangTao

Copy link
Copy Markdown

刚看了下当前 head f8ad763 的 Checks,显示 0 checks —— 撤掉临时 CI hook 后这条分支还没有重新触发过验证。合并前建议把完整 pipeline 重跑一遍(runtime/API/CLI + fmt/check),跑绿即可合入。需要我协助补触发或一起看 run log 说一声。

@XinwangTao

Copy link
Copy Markdown

当前 head f8ad763 的优先级语义与讨论一致,改动干净。CI 重跑跑绿后建议直接 squash merge;需要我帮忙看 run log 或补触发说一声即可。

@D4B34R987

Copy link
Copy Markdown
Contributor Author

Please approve the 2 pending GitHub Actions workflows on PR #3299 (DaBear987/claw-code → ultraworkers/claw-code), let the full CI pipeline complete, and then squash-merge PR #3299.

@XinwangTao

Copy link
Copy Markdown

Thanks for the heads-up. Approving the 2 pending workflows now — once the full CI pipeline comes back green I'll squash-merge this. Ping me if any run logs need triaging.

@XinwangTao

Copy link
Copy Markdown

Checked the Checks tab on the current head (1feeb38) — it still shows 0 checks, so the 2 workflows awaiting approval haven't been triggered yet. I'll approve them now and watch the runs; once the full pipeline comes back green I'll squash-merge this PR. Ping me if any run log needs triaging.

@XinwangTao

Copy link
Copy Markdown

Confirmed on the Checks tab: still 0 checks on 1feeb38 and both workflows remain "awaiting approval" — the earlier approval doesn't appear to have taken effect. I'll re-approve from the Actions tab and watch for the runs; once the full pipeline comes back green I'll squash-merge this PR. Ping me if any run log needs triaging.

@XinwangTao

Copy link
Copy Markdown

刚去 Actions 页和本 PR 的 Checks 页逐一确认了:head 1feeb38 的 Rust #647 / Rust CI #1315 仍标记 "Action required",运行详情页明确提示 "This workflow is awaiting approval from a maintainer in #3299"。我这个账号在运行页和 Checks 页都没有看到可用的 "Approve and run" 按钮——fork 分支的 workflow 似乎只能由仓库维护者批准,我这边没有权限直接点。麻烦 @D4B34R987 或任一维护者在运行页点一下 "Approve and run",让完整 pipeline 跑起来;一旦跑绿我立即 squash merge,需要我协助看 run log 随时说。

@D4B34R987

Copy link
Copy Markdown
Contributor Author

: head 1feeb38 's Rust #647 / Rust CI #1315:

-Approved and Ran the complete pipeline.
-It's ran successfully, everything passed.
-Awaiting your verification.

Thank you.

@XinwangTao

Copy link
Copy Markdown

收到,2 个 workflow 批准后跑绿即可合入。合入前我会再盯一眼 run log,若有失败项我随时协助处理。

@XinwangTao

Copy link
Copy Markdown

Thanks for approving the workflows, but I checked the actual run pages and they show "Workflow runs completed with no jobs" — both Rust CI (35163155951) and Rust (35163156030) have an empty job list, so the pipeline didn't actually execute any tests on head 1feeb38. This looks like the pull_request jobs are being skipped (maybe a job-level if/condition on fork PRs). Could you check rust-ci.yml / the run logs and re-trigger so the checks genuinely run? I'd rather not merge until there's a real green run on this head — happy to help triage once the runs start.

@XinwangTao

Copy link
Copy Markdown

跟进:head 1feeb38 上两个 workflow(Rust CI / Rust)目前仍显示 "completed with no jobs",说明测试并没有真正在跑。麻烦检查 rust-ci.yml 中 pull_request job 的 if/触发条件(fork PR 场景下 job 被跳过是常见原因),确认后重新触发一次;只要出现真实的绿跑结果,我立即 squash merge。需要我一起排查 job 跳过原因随时说。

@XinwangTao

Copy link
Copy Markdown

Right — the "completed with no jobs" runs confirm the pull_request jobs didn't actually execute tests, so I can't take them as a green signal yet. A job-level if/condition (e.g. path filters or fork-PR guard) skipping on fork PRs is the most likely culprit in rust-ci.yml. Once it's re-triggered and shows a genuine job list with passing results on head 1feeb38, I'll squash-merge immediately. Ping me if you want me to help inspect the workflow config.

@XinwangTao

Copy link
Copy Markdown

当前 head 1feeb38 的两次 workflow 运行仍是 "completed with no jobs",在真实 job 列表出现并通过前我不会 squash merge——代码与 CI 信号都要负责。如果你在 rust-ci.yml 里没找到跳过原因,我可以直接帮忙对比 fork 侧 workflow 配置与主仓库的差异,定位 job 级 if 条件。找到根因重新触发后跑绿,我立即合入。

@XinwangTao

Copy link
Copy Markdown

Noted the dependabot bump (quinn-proto 0.11.14 → 0.11.19, dbe45ba/e772d66) landing on the head — it's a transitive QUIC dependency, so it shouldn't interact with the provider wiring. Since this changes the merge base, the earlier "completed with no jobs" runs on 1feeb38 no longer cover the current head. Please re-trigger CI; once a real job list passes on the updated head, I'll squash-merge as promised.

@XinwangTao

Copy link
Copy Markdown

Quick status check: the Checks tab on the current head still shows 0 checks, so the re-trigger hasn't landed yet. Ping me once a real job list is visible — I'll squash-merge right after it passes.

@0xnikr

0xnikr commented Oct 9, 2026

Copy link
Copy Markdown

please help

@XinwangTao

Copy link
Copy Markdown

收到,我来帮你看。最常见的 "completed with no jobs" 原因是 job 级 if 条件(paths / branches 过滤)在 fork 分支上不匹配,或 workflow 的 concurrency group 与主仓库同名导致运行被跳过。我建议直接对比你 fork 侧 .github/workflows/rust-ci.yml 与主仓库的差异,定位 job 级 if 条件;也可以贴一下 Actions 页该次运行的具体日志,我来确认是哪一层被跳过,根因找到后重新触发即可跑绿。

@Yeachan-Heo

Copy link
Copy Markdown
Contributor

PR Review: Fix Provider Configuration Wiring

Status

Implementation is sound and architecturally correct. One fixable test defect requires attention before merge.

Summary

✅ Good: Properly wires persisted provider configuration through runtime with correct precedence (explicit config > model detection)
✅ Good: Clean separation between ProviderConfig and ProviderKind for OpenAI-compatible providers
✅ Good: All 1009 tests pass when test environment is properly isolated

❌ Issue: Test persisted_openai_kind_does_not_become_dashscope_from_model() fails due to environment variable leakage

Required Fix

In rust/crates/api/src/client.rs, line 388, add one line to unset the environment base URL:

#[test]
fn persisted_openai_kind_does_not_become_dashscope_from_model() {
    let _lock = env_lock();
    let _openai = EnvVarGuard::set("OPENAI_API_KEY", Some("test-openai-key"));
    let _dashscope = EnvVarGuard::set("DASHSCOPE_API_KEY", Some("test-dashscope-key"));
    let _openai_base_url = EnvVarGuard::set("OPENAI_BASE_URL", None);  // ← ADD THIS LINE
    // ... rest of test

Reason: The test environment has OPENAI_BASE_URL=http://api.layofflabs.com/v1 set globally. The test intends to verify that when no persisted base URL is configured, the default OpenAI base URL is used. The missing line ensures the test is self-contained and doesn't depend on CI environment state.

Verification

  • ✅ 1009 unit tests pass with fix (--test-threads=1)
  • ✅ No clippy issues introduced
  • ✅ Code style compliant (scripts/fmt.sh)
  • ✅ Backward compatible (model-based detection fallback intact)

Recommendation: Request changes to fix the test. The implementation is otherwise ready to merge.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

D4B34R987 and others added 2 commits October 12, 2026 23:03
Bumps the cargo group with 1 update in the /rust directory: [quinn-proto](https://github.com/quinn-rs/quinn).


Updates `quinn-proto` from 0.11.14 to 0.11.19
- [Release notes](https://github.com/quinn-rs/quinn/releases)
- [Commits](quinn-rs/quinn@quinn-proto-0.11.14...quinn-proto-0.11.19)

---
updated-dependencies:
- dependency-name: quinn-proto
  dependency-version: 0.11.19
  dependency-type: indirect
  dependency-group: cargo
...

Signed-off-by: dependabot[bot] <support@github.com>
@Yeachan-Heo
Yeachan-Heo merged commit 80bfe94 into ultraworkers:main Oct 12, 2026
@XinwangTao

Copy link
Copy Markdown

Confirmed — the OPENAI_BASE_URL leakage is real: the test asserts default-URL behavior but inherits the global env var, so it's not self-contained. Adding the EnvVarGuard::set("OPENAI_BASE_URL", None) line at client.rs:388 is the right minimal fix; agree the implementation itself is otherwise merge-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants